お問い合わせ一覧・個別ページを作成 #7387
お問い合わせ一覧・個別ページを作成 #7387
Conversation
a-kuroki-gs
force-pushed
the
feature/add-inquiry-index-page
branch
from
60d29f2
to
3b29a13
Compare
|
@machida |
|
@a-kuroki-gs お待たせしました🙇♂️ デザインの調整を入れましたー |
| .card-list-item__row | ||
| .a-meta | ||
| p | ||
| = inquiry.body.truncate(100) |
There was a problem hiding this comment.
@machida
デザイン入れていただき、ありがとうございます！
１点こちらのコードについて確認なのですが、
Issueでは、「一覧画面においてメール本文は200字程度表示する」仕様になっていたかと思います。
こちらデザインに合わせて仕様を修正した、という認識でよろしいでしょうか？
There was a problem hiding this comment.
@a-kuroki-gs すいません、伝え忘れてました🙇♂️ ここは一旦100文字に変更でお願いします。思ったより200文字が多かった...
There was a problem hiding this comment.
@machida
承知しました！ご連絡ありがとうございます🙏
|
お疲れ様です！ ご確認よろしくお願いいたします🙏 |
|
わかりました〜明日から時間をとって確認させていただきます！ |
| = link_to admin_inquiry_path(inquiry), class: 'card-list-item-title__link a-text-link' do | ||
| | #{inquiry.name} 様 （#{inquiry.email}） |
There was a problem hiding this comment.
Issue上のやりとりや、コミットメッセージを拝読すると、index上の個別のカード全体がクリックできるような実装にする流れだったようですが、実際はこのタイトル部分にあたる要素のみがリンクになっているように見えます。
赤枠で囲ったあたりがリンクになっている感じです。
こちらが意図なされた動きなのかが気になりました🙏
There was a problem hiding this comment.
📝 こちらミーティングで確認したところ、この動きで良いとのことでした。
| .page-content-header__end | ||
| .page-content-header__row | ||
| h1.page-content-header__title | ||
| | #{@inquiry.name}（#{@inquiry.email}） |
There was a problem hiding this comment.
細かいのですが、indexの個別のカード方では#{inquiry.name} 様 （#{inquiry.email}）になっているので、こちらもその表示に合わせてもいいのかなと思いました👀
（machidaさんが手を入れられたところだと思うので、確認の上での対応になるかもですが）
その上で、もしindexと共通の表示にする場合は、この部分はDecoratorでメソッドにして使い回すのが良いかなと思いました。
There was a problem hiding this comment.
個別と一覧での表示を合わせることにしました。
またご指摘いただいたように、
その上でDecoratorを新たに作成し、テストも追加しております👍
test/system/admin/inquiries_test.rb
Outdated
|
|
||
| test 'show inquiry details' do | ||
| visit_with_auth '/admin/inquiries', 'komagata' | ||
| all('.card-list-item__inner')[0].click |
There was a problem hiding this comment.
別でコメントした「タイトル部分のみがリンクになっていること」と絡むのですが、現状のテストの場合、ここで詳細ページへのリンクを踏めていないと思います。
（パスしているのは、結果的にindex上でもアサーションできているからだと思います。）
もしタイトル部分のみをリンクにするのであれば、テストの意義の観点から、ここでクリックする要素を変更して、正しく詳細ページに遷移するようにすべきかな思います👀
There was a problem hiding this comment.
以下のように、クリックできる要素でテストするよう修正しました。
test/fixtures/inquiries.yml
Outdated
| inquiry1: | ||
| name: 'inquire1' | ||
| email: '[email protected]' | ||
| created_at: '2017-01-01 00:00:00' | ||
| body: 'お問い合わせのテスト1です。' | ||
|
|
||
| inquiry2: | ||
| name: 'inquire2' | ||
| email: '[email protected]' | ||
| created_at: '2017-01-01 00:00:00' | ||
| body: 'お問い合わせのテスト2です。' |
There was a problem hiding this comment.
開発環境にも同じように初期データがあるといいかもと思いました👍
There was a problem hiding this comment.
開発環境にもデータを追加し、inquireとなっていたスペルミスをinquiryに修正しました。
a-kuroki-gs
force-pushed
the
feature/add-inquiry-index-page
branch
from
6c9942e
to
0e32b0c
Compare
|
@junohm410 |
| def index | ||
| @inquiries = Inquiry.all | ||
| end |
There was a problem hiding this comment.
Inquiry.allだと順番が保証されないので、Inquiry.order(created_at: :desc)にした方が良さそうだと思いましたがいかがでしょうか？
（updated_atでソートしている他のリソースもありますが、お問い合わせは現状更新されることはなさそうなのでcreated_atでいいと考えました。）
こちら、本来は前回のレビューで気づくべきでした🙇♂️申し訳ないです。
There was a problem hiding this comment.
@junohm410
ありがとうございます！
確かにおっしゃる通り、allではなく順序を指定して取得すべきでした。
（
updated_atでソートしている他のリソースもありますが、お問い合わせは現状更新されることはなさそうなのでcreated_atでいいと考えました。）
こちらに関しては私もcreated_atを基準にする方針で良いと思います。
気付けなかった点でしたので、ご指摘いただけて助かりました！ありがとうございます。
再度修正しておりますので確認よろしくお願いいたします👍
|
@junohm410 @komagata |
| # frozen_string_literal: true | ||
|
|
||
| module InquiryDecorator | ||
| def sender_name_and_email |
|
|
||
| class InquiryDecoratorTest < ActiveDecoratorTestCase | ||
| setup do | ||
| @inquiry = decorate(inquiries(:inquiry1)) |
test/system/admin/inquiries_test.rb
Outdated
| assert_text inquiries(:inquiry1).body | ||
| end | ||
|
|
||
| test 'click on the link of back to inquiries index' do |
There was a problem hiding this comment.
system testはとても重い時間のかかる処理なので、リンクのテストとかはしなくて大丈夫です～。
There was a problem hiding this comment.
@komagata
リンクのテストを削除しております。
ご確認よろしくお願いいたします🙏
a-kuroki-gs
force-pushed
the
feature/add-inquiry-index-page
branch
2 times, most recently
from
598cb59
to
a25728e
Compare
|
|
||
| class Admin::InquiriesController < AdminController | ||
| def index | ||
| @inquiries = Inquiry.order(created_at: :desc) |
There was a problem hiding this comment.
orderをちゃんとしていしていて、created_atにしてるのがよいとおもいます～！
| span.a-meta__value | ||
| = l inquiry.created_at |
There was a problem hiding this comment.
| span.a-meta__value | |
| = l inquiry.created_at | |
| span.a-meta__value = l inquiry.created_at |
短い行は１行でいいかもです。
| p | ||
| = inquiry.body.truncate(100) |
There was a problem hiding this comment.
| p | |
| = inquiry.body.truncate(100) | |
| p = inquiry.body.truncate(100) |
| h1.page-header__title | ||
| = title |
There was a problem hiding this comment.
| h1.page-header__title | |
| = title | |
| h1.page-header__title = title |
| h1.page-main-header__title | ||
| | お問い合わせ一覧 |
There was a problem hiding this comment.
| h1.page-main-header__title | |
| | お問い合わせ一覧 | |
| h1.page-main-header__title お問い合わせ一覧 |
| span.a-meta__label | ||
| | 受信日 | ||
| span.a-meta__value | ||
| = l @inquiry.created_at |
There was a problem hiding this comment.
| span.a-meta__label | |
| | 受信日 | |
| span.a-meta__value | |
| = l @inquiry.created_at | |
| span.a-meta__label 受信日 | |
| span.a-meta__value = l @inquiry.created_at |
お問い合わせのタブが増えたので、その分を修正
一覧画面のViewにのみリンクを追加 要素を正しくネストさせるために、共通しているコードもひとまず分割する
共通部分のファイル名は_inquiry_commonにする
decoratorのテストも追加
a-kuroki-gs
force-pushed
the
feature/add-inquiry-index-page
branch
from
29da23b
to
056f3c7
Compare
Issue
概要
管理ページにお問い合わせ一覧ページ・詳細ページを追加。
変更確認方法
feature/add-inquiry-index-pageをローカルに取り込むforeman start -f Procfile.devでアプリを起動するお問い合わせのタブがあることお問い合わせのタブをクリックすると、お問い合わせ一覧画面にお問い合わせが表示されていることお問い合わせ一覧へをクリックすると、お問い合わせ一覧画面に遷移することScreenshot
変更前
変更後